Matter Switch: Update table emptiness checks - #3223
Conversation
|
Channel deleted. |
Test Results 73 files 541 suites 0s ⏱️ Results for commit db0ec25. ♻️ This comment has been updated with latest results. |
|
matter-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against db0ec25 |
cjswedes
left a comment
cjswedes
left a comment
There was a problem hiding this comment.
I dont fully understand why this change is necessary. The fields we are storing should be either exclusively using the array portion of the table or we should assume they could be using the hash portion. It seems like for this driver, we need to assume there could be a hash portion because the endpoints could be out of order; we should use pairs. If the fields are a list, we can use table.remove and rely on the # operator.
@cjswedes This change is mostly required to add the table.remove operator. Everything else is not technically necessary, but I do think the # operator has drawbacks that make me lean towards not using it as a generic function (hence why I now added an is_field_empty fn). I agree it would be fine now, but I still would prefer not to use it since it is unreliable in a handful of cases. As for pairs vs. ipairs, this isn't a huge difference anyway but I see what you mean. So seems to me that your main concern is why we wouldn't use the # operator, considering what the shape of the field appears to be otherwise? I suppose I'd say it would purely be to keep things more generic, but I'm open to other ideas. |
|
@cjswedes Read your message again, to more completely answer your question, the actual bug this fixes is that the previous "remove index from table" logic simply set the index to nil, and did not call table.remove. This lead to a case where the # operator saw the table as being empty when it actually wasn't. |
| end | ||
| end | ||
| if #set_topology_eps == 0 then -- in other words, all AvailableEndpoints attribute responses have been handled | ||
| if switch_utils.is_field_empty(device, fields.ELECTRICAL_SENSOR_EPS) then |
There was a problem hiding this comment.
IMO this helper actually makes understanding the layout of the field less clear. If we know this is a list and can use ipairs, then # is sufficient.
82a207b to
db0ec25
Compare
hcarter-775
changed the title
3 participants
Description of Change
Update method for checking table emptiness check to check whether next() is nil, to avoid issues with the '#' operator, which is invalid if keys are not ordered. Also, add and update some logging to be more useful via hub log methods.
Summary of Completed Tests
Tests added to check this works as expected, specifically for devices reporting endpoints out of order.
Tested on a physical TP-Link device that reports its endpoints out of order.